fix: reject vault-escaping paths in VaultWriter - #13
Open
arimu1 wants to merge 1 commit into
Open
Conversation
createNode, annotateNode, and addLink joined user-supplied directory, nodeId, and sourceId values onto vaultPath with plain path.join(), which does not stop `../` segments or absolute overrides from resolving outside the vault. Add resolveInVault(), which resolves the path and rejects it if path.relative(vaultPath, absPath) escapes (starts with '..' or is itself absolute), and route every read/write path in VaultWriter through it. Fixes obra#5 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #5
Root cause
VaultWriter.createNode,annotateNode, andaddLinkinsrc/lib/writer.tsbuild absolute paths withpath.join(this.vaultPath, ...)from user-supplieddirectory/title(createNode),nodeId(annotateNode), andsourceId(addLink) with no check that the result stays insidevaultPath.path.joinnormalizes..segments but does not stop them from resolving outside the vault, so a crafted MCP tool call (e.g.directory: "../../etc"ornodeId: "../../etc/passwd") could create or append to files outside the intended vault directory.Fix
Added a private
resolveInVault(relPath)helper that resolves the path withpath.resolve, computespath.relative(vaultPath, absPath), and throwsPath escape attempt: <path>if the relative path starts with..or is itself absolute. Every read/write entry point inVaultWriter(createNode,annotateNode,addLink, and the internalindexFile) now routes through this helper instead of rawpath.join. Normal in-vault paths are unaffected.Testing
Added traversal-rejection tests to
test/writer.test.ts:createNoderejects adirectoryof../../etccreateNoderejects atitleof../../../tmp/evilannotateNoderejects anodeIdof../../etc/passwdaddLinkrejects asourceIdof../../etc/passwdAll existing tests continue to pass with normal (non-escaping) paths.
(Full-repo
npm run buildhas pre-existing, unrelated TypeScript errors insrc/lib/graph.ts,src/lib/embedder.ts, andsrc/mcp/index.tson unmodifiedmain— verified by stashing this change and re-runningtsc.writer.tsitself introduces no new type errors.)Drafted with Claude Code (model: Claude Fable 5) and human-reviewed before submission. I verified the vulnerable code paths, wrote/ran the tests above, and confirmed the pre-existing build errors are unrelated to this change.